Clean-up DeviceContext (Remove shadowed data structure) #1962

umariqbal-rs · 2022-01-31T12:11:45Z

Description

In this PR we will Clean-up DeviceContext (Remove shadowed data structure).

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Bug fix (change which fixes an issue)
New feature (change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation
I have updated the documentation accordingly
I have added tests to cover my changes
All new and existing tests passed

tangxifan

@umariqbal-rs I review the codes. For all the client functions, please use RRGraphView instead of RRGraphBuilder when accessing data

tangxifan · 2022-01-31T18:24:47Z

vpr/src/base/read_route.cpp

@@ -126,7 +126,7 @@ bool read_route(const char* route_file, const t_router_opts& router_opts, bool v
    recompute_occupancy_from_scratch();

    /* Note: This pres_fac is not necessarily correct since it isn't the first routing iteration*/
-    OveruseInfo overuse_info(device_ctx.rr_nodes.size());
+    OveruseInfo overuse_info(device_ctx.rr_graph_builder.size());


For overuse report, just need to use RRGraphView

tangxifan · 2022-01-31T18:30:54Z

vpr/src/base/vpr_context.h

@@ -163,7 +163,7 @@ struct DeviceContext : public Context {
    /* A writeable view of routing resource graph to be the ONLY database 
     * for routing resource graph builder functions.
     */
-    RRGraphBuilder rr_graph_builder{&rr_nodes, &rr_node_metadata, &rr_edge_metadata};
+    RRGraphBuilder rr_graph_builder{&rr_node_metadata, &rr_edge_metadata};


Please also delete the line 150 in this file

tangxifan · 2022-01-31T18:33:20Z

vpr/src/device/rr_graph_builder.h

@@ -273,6 +273,10 @@ class RRGraphBuilder {
    inline void init_fan_in() {
        node_storage_.init_fan_in();
    }
+    /** @brief Return number of nodes. This function is inlined for runtime optimization. */
+    inline size_t size() const {


We can use the API name num_nodes() here, we can also change the API name in RRGraphView

tangxifan · 2022-01-31T18:33:43Z

vpr/src/draw/draw.cpp

@@ -929,7 +929,7 @@ void alloc_draw_structs(const t_arch* arch) {
    /* Space is allocated for draw_rr_node but not initialized because we do *
     * not yet know information about the routing resources.				  */
    draw_state->draw_rr_node = (t_draw_rr_node*)vtr::malloc(
-        device_ctx.rr_nodes.size() * sizeof(t_draw_rr_node));
+        device_ctx.rr_graph_builder.size() * sizeof(t_draw_rr_node));


Use RRGraphView here

tangxifan · 2022-01-31T18:33:48Z

vpr/src/draw/draw.cpp

@@ -980,7 +980,7 @@ void init_draw_coords(float width_val) {
        return; //do not initialize only if --disp off and --save_graphics off
    /* Each time routing is on screen, need to reallocate the color of each *
     * rr_node, as the number of rr_nodes may change.						*/
-    if (device_ctx.rr_nodes.size() != 0) {
+    if (device_ctx.rr_graph_builder.size() != 0) {


Use RRGraphView here

tangxifan · 2022-01-31T18:35:11Z

vpr/src/route/route_timing.cpp

@@ -322,7 +322,7 @@ bool try_timing_driven_route_tmpl(const t_router_opts& router_opts,
     */
    bool routing_is_successful = false;
    WirelengthInfo wirelength_info;
-    OveruseInfo overuse_info(device_ctx.rr_nodes.size());
+    OveruseInfo overuse_info(device_ctx.rr_graph_builder.size());


Use RRGraphView here

tangxifan · 2022-01-31T18:35:28Z

vpr/src/route/route_tree_timing.cpp

@@ -80,7 +80,7 @@ bool alloc_route_tree_timing_structs(bool exists_ok) {
    /* Allocates any structures needed to build the routing trees. */

    auto& device_ctx = g_vpr_ctx.device();
-    bool route_tree_structs_are_allocated = (rr_node_to_rt_node.size() == size_t(device_ctx.rr_nodes.size())
+    bool route_tree_structs_are_allocated = (rr_node_to_rt_node.size() == size_t(device_ctx.rr_graph_builder.size())


Use RRGraphView here

tangxifan · 2022-01-31T18:35:33Z

vpr/src/route/router_delay_profiling.cpp

@@ -100,7 +100,7 @@ std::vector<float> calculate_all_path_delays_from_rr_node(int src_rr_node, const
    auto& device_ctx = g_vpr_ctx.device();
    auto& routing_ctx = g_vpr_ctx.mutable_routing();

-    std::vector<float> path_delays_to(device_ctx.rr_nodes.size(), std::numeric_limits<float>::quiet_NaN());
+    std::vector<float> path_delays_to(device_ctx.rr_graph_builder.size(), std::numeric_limits<float>::quiet_NaN());


Use RRGraphView here

tangxifan · 2022-01-31T18:35:37Z

vpr/src/route/router_lookahead_extended_map.cpp

@@ -438,8 +438,8 @@ void ExtendedMapLookahead::compute(const std::vector<t_segment_inf>& segment_inf
        util::RoutingCosts delay_costs;
        util::RoutingCosts base_costs;
        int total_path_count = 0;
-        std::vector<bool> node_expanded(device_ctx.rr_nodes.size());
-        std::vector<util::Search_Path> paths(device_ctx.rr_nodes.size());
+        std::vector<bool> node_expanded(device_ctx.rr_graph_builder.size());


Use RRGraphView here

tangxifan · 2022-01-31T18:35:41Z

vpr/src/route/router_lookahead_map.cpp

@@ -588,7 +588,7 @@ static void run_dijkstra(RRNodeId start_node, int start_x, int start_y, t_routin
    const auto& rr_graph = device_ctx.rr_graph;

    auto& node_expanded = data->node_expanded;
-    node_expanded.resize(device_ctx.rr_nodes.size());
+    node_expanded.resize(device_ctx.rr_graph_builder.size());


Use RRGraphView here

umariqbal-rs · 2022-02-02T05:11:40Z

@tangxifan
Can you Have a look on my PR?
Thanks

tangxifan

@umariqbal-rs CI is red on basic tests. Can you look into it?

tangxifan · 2022-02-02T05:28:11Z

vpr/src/route/spatial_route_tree_lookup.cpp

@@ -78,7 +78,7 @@ size_t grid_to_bin_y(size_t grid_y, const SpatialRouteTreeLookup& spatial_lookup
 bool validate_route_tree_spatial_lookup(t_rt_node* rt_node, const SpatialRouteTreeLookup& spatial_lookup) {
    auto& device_ctx = g_vpr_ctx.device();
    const auto& rr_graph = device_ctx.rr_graph;
-    auto& rr_node = device_ctx.rr_nodes[rt_node->inode];
+    auto& rr_node = rr_graph.rr_nodes()[rt_node->inode];


Since we have finished all the APIs, you can consider to replace this line by

RRNodeId rr_node = rt_node->inode;

And also adapt the rest codes in this function, rr_node.id() -> rr_node

tangxifan · 2022-02-02T05:28:40Z

vpr/src/route/spatial_route_tree_lookup.cpp

@@ -33,7 +33,7 @@ void update_route_tree_spatial_lookup_recur(t_rt_node* rt_node, SpatialRouteTree
    auto& device_ctx = g_vpr_ctx.device();
    const auto& rr_graph = device_ctx.rr_graph;

-    auto& rr_node = device_ctx.rr_nodes[rt_node->inode];
+    auto& rr_node = rr_graph.rr_nodes()[rt_node->inode];


Since we have finished all the APIs, you can consider to replace this line by

RRNodeId rr_node = rt_node->inode;

And also adapt the rest codes in this function, rr_node.id() -> rr_node

tangxifan · 2022-02-02T05:31:05Z

vpr/src/route/rr_graph_indexed_data.cpp

@@ -451,7 +451,7 @@ static void load_rr_indexed_data_T_values() {
 static void calculate_average_switch(int inode, double& avg_switch_R, double& avg_switch_T, double& avg_switch_Cinternal, int& num_switches, short& buffered, vtr::vector<RRNodeId, std::vector<RREdgeId>>& fan_in_list) {
    auto& device_ctx = g_vpr_ctx.device();
    const auto& rr_graph = device_ctx.rr_graph;
-    const auto& rr_nodes = device_ctx.rr_nodes.view();
+    const auto& rr_nodes = rr_graph.rr_nodes().view();


I do not think we still need a view here. Can you try to delete this line? For LINE 466, you can use the rr_graph.
If it does work, we keep this line.

tangxifan · 2022-02-02T05:35:30Z

vpr/src/device/rr_graph_view.h

+    // Returns a range of RREdgeId's belonging to RRNodeId id.
+    //
+    // If this range is empty, then RRNodeId id has no edges.
+    inline vtr::StrongIdRange<RREdgeId> edge_range(const RRNodeId id) const {


const RRNodeId -> RRNodeId
It will be more memory efficient.

tangxifan · 2022-02-02T05:36:06Z

vpr/src/device/rr_graph_view.h

    /* -- Internal data storage -- */
    /* Note: only read-only object or data structures are allowed!!! */
  private:
    /* node-level storage including edge storages */
    const t_rr_graph_storage& node_storage_;
    /* Fast look-up for rr nodes */
    const RRSpatialLookup& node_lookup_;
-
+    const vtr::vector<RREdgeId, RRNodeId> edge_dest_node_;


Can you explain why these two data are required?

tangxifan · 2022-02-02T05:37:08Z

vpr/src/base/read_route.cpp

@@ -505,9 +505,9 @@ static bool check_rr_graph_connectivity(RRNodeId prev_node, RRNodeId node) {
    if (prev_node == node) return false;

    auto& device_ctx = g_vpr_ctx.device();
-    const auto& rr_graph = device_ctx.rr_nodes;
+    const auto& rr_graph = device_ctx.rr_graph;
    /*TODO We need to remove temp_rr_graph once rr_graph is resolved*/


Just remove these two lines.

tangxifan · 2022-02-02T05:37:29Z

vpr/src/base/vpr_context.h

@@ -147,7 +147,7 @@ struct DeviceContext : public Context {
    /*
     * Structures to define the routing architecture of the FPGA.
     */
-    t_rr_graph_storage rr_nodes; // autogenerated in build_rr_graph
+    //t_rr_graph_storage rr_nodes; // autogenerated in build_rr_graph


Remove this line

umariqbal-rs · 2022-02-02T05:48:53Z

vpr/src/device/rr_graph_view.h

    /** @brief Get the destination node for the iedge'th edge from specified RRNodeId.
     *  This method should generally not be used, and instead first_edge and
     *  last_edge should be used.*/
    inline RRNodeId edge_sink_node(RRNodeId id, t_edge_size iedge) const {
        return node_storage_.edge_sink_node(id, iedge);
    }
+    inline RRNodeId edge_sink_node(RREdgeId edge) const {
+        return edge_dest_node_[edge];


@tangxifan
We are using it here const vtr::vector<RREdgeId, RRNodeId> edge_dest_node_;.
to replace here.

vtr-verilog-to-routing/vpr/src/route/route_common.cpp

Line 1641 in 01945e9

auto switch_id = rr_graph.edge_switch(prev_edge);

tangxifan · 2022-02-02T07:14:47Z

vpr/src/route/route_common.cpp

@@ -568,7 +568,7 @@ static t_trace_branch traceback_branch(int node, int target_net_pin_index, std::
        t_trace* prev_ptr = alloc_trace_data();
        prev_ptr->index = inode;
        prev_ptr->net_pin_index = OPEN; //Net pin index is invalid for Non-SINK nodes
-        prev_ptr->iswitch = device_ctx.rr_nodes.edge_switch(iedge);
+        prev_ptr->iswitch = rr_graph.edge_switch(iedge);


Does rr_graph.nodes().edge_switch(iedge) work here? If we can reuse, it is better to reuse.

@tangxifan
I am facing some unknown errors for failing of basics tests. I will see it now.

umariqbal-rs · 2022-02-02T07:25:36Z

vpr/src/draw/draw.cpp

@@ -2288,7 +2288,7 @@ static void draw_rr_pin(int inode, const ezgl::color& color, ezgl::renderer* g)
 * the physical pin is on.                                                  */
 void draw_get_rr_pin_coords(int inode, float* xcen, float* ycen, const e_side& pin_side) {
    auto& device_ctx = g_vpr_ctx.device();
-    draw_get_rr_pin_coords(device_ctx.rr_nodes[inode], xcen, ycen, pin_side);
+    draw_get_rr_pin_coords(device_ctx.rr_graph.rr_nodes()[inode], xcen, ycen, pin_side);


@tangxifan
Can you Please Have a look on this line ?

@umariqbal-rs This line looks o.k. We will refactor the functions in the draw.cpp later. Ideally, we want these function to use rr_graph and RRNodeId, rather than sending a rr_node data structure.

umariqbal-rs · 2022-02-02T13:07:14Z

@tangxifan
Can you Please have a look my PR?

tangxifan

@umariqbal-rs Code changes are clean! Thanks for the hard working. Can you start running QoR tests?

umariqbal-rs · 2022-02-03T12:06:45Z

@tangxifan
Below are the QoR Results with Current master:
vtr_reg_qor_chain_depop test:
Flow run time 0.04% on average.
critical path delay 0% on average.
Peak memory usage 0.01% on average
titan_quick_qor test:
flow run time 14.48% on average.
critical path delay 0% on average.
Peak memory usage 0% on average
vtr_reg_qor_chain_depop.xlsx
titan_quic_qor.xlsx

umariqbal-rs · 2022-02-03T12:11:27Z

@tangxifan
Below are the QoR Results with Sanity:
vtr_reg_qor_chain_depop test:
Flow run time -2.11% on average.
titan_quick_qor test:
flow run time 10.9% on average.

tangxifan · 2022-02-03T17:13:46Z

vpr/src/device/rr_graph_view.h

@@ -419,6 +429,10 @@ class RRGraphView {
    const RRSpatialLookup& node_lookup() const {
        return node_lookup_;
    }
+    /** @brief Return the node-level storage structure for queries from client functions */
+    const t_rr_graph_storage& rr_nodes() const {


I am aware of the runtime overhead. Can you try to make this API inline?

tangxifan · 2022-02-03T17:31:23Z

@umariqbal-rs I see the runtime overhead is mainly on packer (+25% on average), while placer is next (+9% on average) and router is least (+7%)

tangxifan · 2022-02-03T19:08:59Z

@umariqbal-rs

@vaughnbetz and I have gone through the spreadsheets. We see a constant overhead on runtime across all the Titan benchmarks. To help us understand why, can you try

Use an isolated machine where we can avoid the server workload issues
Try 1 or 2 benchmarks from Titan benchmarks on the selected machine
Check if you can still see any runtime difference (if we see the runtime difference in packer insists, we should pay attention).

Before this happen, ensure

We have inline the APIs

umariqbal-rs · 2022-02-04T03:59:32Z

@tangxifan
Sure I will do this. I will do this in evening time when load is less on server.

tangxifan · 2022-02-04T18:20:37Z

@umariqbal-rs Remember to resolve the merging conflicts

umariqbal-rs · 2022-02-08T06:03:58Z

@tangxifan
Below are the QoR Results with Current master:
vtr_reg_qor_chain_depop test:
Flow run time 0.84% on average.
critical path delay 0% on average.
Peak memory usage 0% on average
titan_quick_qor test:
flow run time -0.57% on average.
critical path delay 0% on average.
Peak memory usage 0% on average
vtr_reg_chain_depop.xlsx
titan_quick_qor_master.xlsx

tangxifan

@umariqbal-rs I see some changes in the download_titan.py script. Is it intended?

umariqbal-rs · 2022-02-08T06:51:49Z

@umariqbal-rs I see some changes in the download_titan.py script. Is it intended?

@tangxifan
Yes Actually for running titan_tests I have previously used down_titan.py file which you provided.

tangxifan · 2022-02-08T07:24:57Z

@umariqbal-rs I see some changes in the download_titan.py script. Is it intended?

@tangxifan Yes Actually for running titan_tests I have previously used down_titan.py file which you provided.

Emm. Weird. I cannot see this file in the change list. If the down_titan.py file is even with the current master. It should be o.k. then

umariqbal-rs · 2022-02-08T07:29:41Z

@umariqbal-rs I see some changes in the download_titan.py script. Is it intended?

@tangxifan Yes Actually for running titan_tests I have previously used down_titan.py file which you provided.

Emm. Weird. I cannot see this file in the change list. If the down_titan.py file is even with the current master. It should be o.k. then
@tangxifan
I had used download_titan.py that you had sent us earlier to run test locally and pushed it on github, now I have restored download_titan.py.

umariqbal-rs · 2022-02-09T04:53:25Z

@tangxifan
Kokoro tests are not running on PR.

tangxifan · 2022-02-09T06:08:10Z

@tangxifan Kokoro tests are not running on PR.

No worries. We have logged this in #1965

tangxifan

@umariqbal-rs Thanks for the hard working. It seems that the inline keyword impacts a lot here.

@vaughnbetz I propose to merge. QoR tests have been rerun on an isolated machine which shows no performance degradation. If you see anything wrong, feel free to comment.

umariqbal-rs requested a review from tangxifan January 31, 2022 12:12

github-actions bot added the VPR VPR FPGA Placement & Routing Tool label Jan 31, 2022

tangxifan requested changes Jan 31, 2022

View reviewed changes

sfkhalid mentioned this pull request Jan 31, 2022

Kokoro tests not running on pull requests #1963

Closed

behzadmehmood-rs assigned umariqbal-rs Feb 1, 2022

umariqbal-rs requested a review from tangxifan February 2, 2022 04:42

tangxifan requested changes Feb 2, 2022

View reviewed changes

umariqbal-rs commented Feb 2, 2022

View reviewed changes

tangxifan reviewed Feb 2, 2022

View reviewed changes

umariqbal-rs commented Feb 2, 2022

View reviewed changes

umariqbal-rs closed this Feb 2, 2022

github-actions bot removed the VPR VPR FPGA Placement & Routing Tool label Feb 2, 2022

rr_nodes done

4cdcefd

umariqbal-rs reopened this Feb 2, 2022

github-actions bot added the VPR VPR FPGA Placement & Routing Tool label Feb 2, 2022

tangxifan reviewed Feb 2, 2022

View reviewed changes

behzadmehmood-rs added the kokoro:force-run label Feb 3, 2022

symbiflow-robot removed the kokoro:force-run label Feb 3, 2022

tangxifan reviewed Feb 3, 2022

View reviewed changes

Inlined the API

65f6483

tangxifan mentioned this pull request Feb 4, 2022

Dashboard for RRGraph API refactoring status #1868

Open

44 tasks

conf resolved

ccd6817

umariqbal-rs added 3 commits February 7, 2022 18:59

format

2132e81

conflicts resolved

5255f20

download_titan.py

42e492d

tangxifan requested changes Feb 8, 2022

View reviewed changes

tangxifan added the kokoro:force-run label Feb 8, 2022

symbiflow-robot removed the kokoro:force-run label Feb 8, 2022

behzadmehmood-rs added the kokoro:force-run label Feb 9, 2022